Add new copy dialog when there are save issues detected (#3637)#3754
Add new copy dialog when there are save issues detected (#3637)#3754endrift merged 2 commits intomgba-emu:masterfrom
Conversation
|
With pleasure! 👀 |
src/platform/qt/CoreManager.cpp
Outdated
| QMessageBox error; | ||
| error.setIcon(QMessageBox::Critical); | ||
| error.setWindowTitle(title); | ||
| error.setText(tr("%1 Would you like to move the ROM to a different location? If you don't, this will likely lead to data loss (e.g. saves, screenshots, etc.).").arg(summary)); |
There was a problem hiding this comment.
Stylistically I would also consider %1\n\n for readability.
There was a problem hiding this comment.
I might also suggest summary + "\n\n" + tr("...") to keep the first line out of the translation, but that's entirely optional.
| if (ok) { | ||
| return newPath; | ||
| } | ||
| } |
There was a problem hiding this comment.
Perhaps attempt to delete the destination file if it was created but not fully populated? I can imagine an edge case where you run into a disk-full condition and the partial ROM is eating up the last megabyte of space on a tiny SD card.
src/platform/qt/CoreManager.cpp
Outdated
| } | ||
| if (bad) { | ||
| QString newPath = saveFailed(vf, tr("Temporary file loaded"), | ||
| tr("The ROM appears to be loaded from a temporary directory, perhaps automatically extracted from an archive."), |
There was a problem hiding this comment.
Considering our userbase, perhaps "archive (e.g. zip file)"
There was a problem hiding this comment.
Yeah, I had tried to figure out how to make that clearer. I guess that works.
src/platform/qt/CoreManager.cpp
Outdated
| QMessageBox retry; | ||
| retry.setIcon(QMessageBox::Critical); | ||
| retry.setWindowTitle(tr("Copy failed")); | ||
| retry.setText(tr("Failed to copy ROM. Do you want to try again?")); |
There was a problem hiding this comment.
The engineer in me would like to be able to report why the copy failed. QIODevice::errorString() might be a good place to start, though you might have better luck with QFileDevice::error() mapping into something where you can control/localize the messages.
There was a problem hiding this comment.
I don't think we need to do that, at least not yet.
src/platform/qt/CoreManager.cpp
Outdated
| } | ||
|
|
||
| auto retry = [this]() { | ||
| QMessageBox retry; |
There was a problem hiding this comment.
I broadly prefer to use QMessageBox::critical() over constructing the object longhand if I'm going to be using exec() instead of open(). Your codebase, your style preference, but I think that's how it's been done elsewhere as well.
There was a problem hiding this comment.
the signature is so long that you kinda have to know what the order is, but it is what I do already elsewhere in the codebase I suppose.
src/platform/qt/CoreManager.cpp
Outdated
| } | ||
|
|
||
| QString CoreManager::saveFailed(VFile* vf, const QString& title, const QString& summary, const QString& filter) { | ||
| QMessageBox error; |
There was a problem hiding this comment.
Ditto what I said below about using the static method.
ahigerd
left a comment
ahigerd
left a comment
There was a problem hiding this comment.
Mostly style comments. One point of functionality you might consider, but I would be okay with merging this as-is.
2 participants
Can you review this @ahigerd?